fix: remove session key auto-revoke, increase MAX_SESSION_KEYS#4
Conversation
…ION_KEYS to 10 In Aztec's UTXO model, atomically revoking existing session key notes and inserting a new one causes nullifier conflicts when the PXE has stale state or a previous TX is still in the mempool. Two TXs cannot consume the same note — this is fundamental, not a sync issue. Changes: - authorize_session_key is now insert-only (no get_notes + remove loop) - MAX_SESSION_KEYS increased from 5 to 10 for orphaned note headroom - Stale notes cleaned up via revoke_session_key (called from client on a best-effort basis before each new authorization) Companion PR: Apertrue/web#24 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change increases Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webauthn_account/src/main.nr (1)
186-209:⚠️ Potential issue | 🟡 MinorPotential edge case: session key lookup may fail if orphaned notes exceed limit.
The insert-only approach is sound for avoiding nullifier conflicts, but there's a correctness edge case: if orphaned notes accumulate beyond
MAX_SESSION_KEYS(10),is_valid_implmay not find a valid session key in the returned note set, causing authentication to fail unexpectedly. Similarly,revoke_session_keymight miss its target.This relies on the companion PR's client-side cleanup being robust. Consider whether the client should track note count and proactively revoke stale keys before reaching the limit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webauthn_account/src/main.nr` around lines 186 - 209, authorize_session_key currently unconditionally inserts a SessionKeyNote which can overflow the MAX_SESSION_KEYS (10) slot window and make is_valid_impl / revoke_session_key miss keys; update authorize_session_key to first read the existing notes at self.storage.session_keys.at(owner), count them, and if count >= MAX_SESSION_KEYS either (a) purge or revoke the oldest/stale notes by selecting candidates using SessionKeyNote.expiry (and invoking revoke_session_key or the storage removal API) before inserting, or (b) reject the new authorization with a clear error/return so callers can retry cleanup; ensure the logic references SessionKeyNote, MAX_SESSION_KEYS, authorize_session_key, is_valid_impl, and revoke_session_key so behavior remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@webauthn_account/src/main.nr`:
- Around line 186-209: authorize_session_key currently unconditionally inserts a
SessionKeyNote which can overflow the MAX_SESSION_KEYS (10) slot window and make
is_valid_impl / revoke_session_key miss keys; update authorize_session_key to
first read the existing notes at self.storage.session_keys.at(owner), count
them, and if count >= MAX_SESSION_KEYS either (a) purge or revoke the
oldest/stale notes by selecting candidates using SessionKeyNote.expiry (and
invoking revoke_session_key or the storage removal API) before inserting, or (b)
reject the new authorization with a clear error/return so callers can retry
cleanup; ensure the logic references SessionKeyNote, MAX_SESSION_KEYS,
authorize_session_key, is_valid_impl, and revoke_session_key so behavior remains
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04a81f0d-adc1-4040-8ab0-bbda4013c8e4
📒 Files selected for processing (1)
webauthn_account/src/main.nr
1 participant
Summary
authorize_session_key()— now insert-only, producing zero nullifiers from existing notesMAX_SESSION_KEYSfrom 5 to 10 — provides headroom for orphaned notes between client-side cleanup cyclesWhy
In Aztec's UTXO model, the
get_notes+remove(each)loop produced nullifiers. When PXE had stale state or another TX was in the mempool, the same nullifiers appeared in two TXs → "Nullifier conflict" rejection. This is fundamental to the UTXO model and cannot be fixed with sync-and-retry.Orphaned notes (from browser refresh losing the in-memory key) are now cleaned up by the client via
revoke_session_key()on a best-effort basis before each new authorization.Companion PR
Apertrue/web#24 — TypeScript changes (stale key tracking, simplified wallet flow)
Deployment
After merge:
./build.sh→ recompile + AVM transpile + copy artifact to web app. Existing devnet accounts need redeployment (new bytecode = new contract class ID).Test plan
nargo compilepassesbuild.shpipeline succeeds (requires sandbox Docker container)revoke_session_keystill works for individual key revocationis_valid_implfinds session key with up to 10 notes in storage🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes